Make the package compatible Node.js and others runtimes #30

riderx · 2024-02-13T02:57:54Z

Hi, really like your package and use it in my project Capgo.app.
My backend is deployed to multi env, Deno, Node.js and Cloudflare workers.
On Node.js, I was using Minio as an alternative, with some conversion script.
Now since I switched to import map I cannot do so.

For a while, I copied all your files into my project and removed the reference for Deno lib.

Then I discovered There is a better alternative to Buffer available in every runtime:
https://sindresorhus.com/blog/goodbye-nodejs-buffer
Then I made the script to publish in NPM and published my fork here:
https://www.npmjs.com/package/@capgo/s3-lite-client

I also created a huge PR to make minio sdk compatible with all runtimes, but the change is huge, and even all tests pass I'm not quite sure if this will be merged anytime soon.

All tests passed, and changes are the minimum possible.

Related issue: #26

…ttps://sindresorhus.com/blog/goodbye-nodejs-buffer

riderx · 2024-02-13T03:19:39Z

This package can now be publish in npm with deno run -A --no-check build_npm.ts 0.1.2 and then cd npm && npm publish
Update the version number in the first command to your need

bradenmacdonald

Hey, thanks for this contribution! I appreciate it. I have a few small concerns at the moment, but if we can address them I'm willing to consider publishing it on npm.

bradenmacdonald · 2024-02-13T05:08:54Z

transform-chunk-sizes.test.ts

@@ -9,7 +9,7 @@ import { TransformChunkSizes } from "./transform-chunk-sizes.ts";
 */
 class NumberSource extends ReadableStream<Uint8Array> {
  constructor(delayMs: number, chunksCount: number, bytesPerChunk = 1) {
-    let intervalTimer: number;
+    let intervalTimer: any;


Suggested change

let intervalTimer: any;

let intervalTimer: ReturnType<typeof setTimeout>;

Better to use a specific type.

bradenmacdonald · 2024-02-13T05:16:50Z

transform-chunk-sizes.ts

@@ -1,36 +1,39 @@
-import { Buffer } from "./deps.ts";


Unfortunately something about this changed TransformChunkSizes is causing the integration tests to fail, which means there must be a bug in how it's processing the data.

Then I discovered There is a better alternative to Buffer available in every runtime: https://sindresorhus.com/blog/goodbye-nodejs-buffer

That article is about Node's Buffer. This repo uses the Deno Buffer which explicitly says:

Buffer is NOT the same thing as Node's Buffer. Node's Buffer was created in

2009 before JavaScript had the concept of ArrayBuffers. It's simply a

non-standard ArrayBuffer.

What is wrong with using Deno's Buffer implementation? It doesn't use any Deno-specific APIs as far as I know. If you just leave it in, what doesn't work?

BTW at one point Deno std decided to deprecate their Buffer, so I actually created a branch of this that works without it, by copying its code into this repo: https://github.com/bradenmacdonald/deno-s3-lite-client/compare/no-buffer . But they changed their mind, and so I'm still using Deno std Buffer.

I wasn't sure dnt will make it work, I can try

bradenmacdonald · 2024-02-13T05:17:56Z

build_npm.ts

+    custom: [
+      {
+        package: {
+          name: "web-streams-polyfill",


Question: Can we just import these from node:stream/web which is built into Node.js? That would keep the overall npm package size smaller, which is a goal for this project (small and no third party dependencies outside std lib).

it seems node:stream/web doesn't implement stream the same way:

Hmm, it might be just a type error; I wonder if it works if the type checking is disabled. But in any case, seems like it's not a straightforward change. Thanks for checking into it anyways.

i will try to run integration test

I managed to run integration test in Node.js and this doesn't work, it's not only the type:

Ok in fact integration test fail as well with polyfill

bradenmacdonald · 2024-02-13T05:19:23Z

Please also make sure the formatter and linter issues are fixed: deno fmt and deno lint

riderx · 2024-02-17T01:32:02Z

@bradenmacdonald I managed to make it work !!
Test passed, and the integration test passed as well, I use node:stream/web as you recommended.
I had to add undici shim for the copy function.

riderx · 2024-02-17T01:44:26Z

I published it here temporary:
https://www.npmjs.com/package/@capgo/s3-lite-client

riderx · 2024-02-18T00:12:02Z

So after using my version in Cloudflare worker I found one last issue, in Undici, just one old import who breaks things.
I opened a PR in Undici and I will see if we have a way to prevent that in the build script here until they fix it:
nodejs/undici#2775
The Lib works in Node.js 16+ it's only issue on Cloudflare workers

riderx · 2024-02-18T01:21:08Z

Even if I resolve this issue in local, there is another one after.
So I think I will revert to polyfill

riderx · 2024-02-18T02:53:42Z

Ok, so I finally found the best way possible!
Rewriting the way ReadableStream was used resolved all issues, also switching to node 20 min made Undici not required!
We now have the minimum scope possible

riderx · 2024-02-18T02:57:52Z

Tests result:

riderx · 2024-02-18T02:58:33Z

@bradenmacdonald can you allow the workflow tu run ?

riderx · 2024-02-18T04:39:09Z

Ok Nodejs was working but not wrangler due to a weird bug reported here:
cloudflare/workerd#1711
I found out oak server found his own custom solution with optional shims:
https://github.com/oakserver/oak/blob/main/node_shims.ts
So I did implement it and it worked!
All tests passed and Node.js and Cloudflare worker work now

bradenmacdonald

Thanks for your work here! I've been sick for a few days but I'll try to look at this soon. I would like to play around with this myself to see if there's a way to stick with the ReadableStream.from() method used before, because the code was shorter and more concise that way.

bradenmacdonald · 2024-02-21T21:03:11Z

node_shims.ts

+      },
+    });
+  })();
+}


I think this ^ merging into globals will be done automatically by dnt if you specify it in the DNT config shims.custom:

shims: { deno: { test: "dev", }, custom: [{ package: { name: "node:stream/web", }, globalNames: [ "ReadableStream", "WritableStream", "TransformStream", ], }], },

See denoland/dnt#362 and this integration test which checks that this approach works.

I did it here, and it does work on Node.js env but break in Workers env.
That why I implemented as oak project does (made by a Deno maintainer)

riderx · 2024-02-24T01:23:00Z

My PR in undici got merged, so now if Cloudflare resolve this issue: cloudflare/workerd#1711
we can use dnt instead of my shim file

bradenmacdonald · 2024-03-03T19:41:30Z

@riderx Thanks to Deno's new JSR, I've been able to publish a new version that should work on all runtimes.

https://jsr.io/@bradenmacdonald/s3-lite-client

https://github.com/bradenmacdonald/s3-lite-client#installation

For Node.js, it requires Node 19+, because that's when they added the crypto and ReadableStream etc. globals to Node.js

I've tested it on Deno, Bun, Node 20, and Firefox and it's working on all of them. Would you be able to let me know if it's working on CloudFlare workers?

Thanks for all your help with this :) I did need to incorporate some of your fixes for e.g. ReadableStream.from

riderx · 2024-03-05T13:12:30Z

Nice ! Let me try that today :)

riderx · 2024-03-06T14:18:38Z

So for my test it seems the download url get generated but the format is not correct:

<Error>
  <Code>SignatureDoesNotMatch</Code>
  <Message>The request signature we calculated does not match the signature you provided. Check your secret access key and signing method. </Message>
</Error>

for this url format:
https://9ee3d7479a3c359681e3fab2c8cb22c0.r2.cloudflarestorage.com/capgo/apps/8b595537-9d2d-4674-b7a0-15dcf5f38ed8/com.primeopinion.appname/versions/1.0.173.zip?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=25ce8abb1f6387d2bb3b126b4ded7784/20240306/auto/s3/aws4_request&X-Amz-Date=20240306T141750Z&X-Amz-Expires=604800&X-Amz-SignedHeaders=host&X-Amz-Signature=9a9511b6b9c43946cd4de06b709fdca295c1c88beeb4000917d1222e7fc43a37

bradenmacdonald · 2024-03-06T17:17:09Z

@riderx Hmm, I don't suppose you know what's wrong? Does it work with this/your branch, using the exact same code?

Signature issues are often due to the region setting, a wrong clock (doesn't seem to be the case), or issues with URL encoding such as / vs. %2F in the URL.

riderx · 2024-03-06T18:53:13Z

I double-checked in my fork and same issue.
At least, we reached an equivalent problem 🤣
I will try to understand what is happening.

bradenmacdonald · 2024-05-01T21:09:13Z

@riderx Did you have any luck figuring this out with CloudFlare workers?

In the meantime, I'm going to close this issue as the new version at https://jsr.io/@bradenmacdonald/s3-lite-client is compatible with most runtimes. Feel free to open a new issue for CloudFlare compatibility or any other compatibility bugs you may find.

willsmanley · 2024-10-19T04:54:04Z

so sad to see this epic PR not get merged in despite it being a good thing that JSR solves the problem

however i am having some issues while trying to use pnpm + cloudfare workers

firstly, rather than import { S3Client } from "@bradenmacdonald/s3-lite-client"; i have to use import { Client } from "@bradenmacdonald/s3-lite-client/client"; which is fine but strange. maybe its because no main in package.json?

secondly i can install just fine with pnpm dlx jsr add @bradenmacdonald/s3-lite-client but then when adding another package like pnpm add dotenv it blocks with:

 ERR_PNPM_FETCH_404  GET https://registry.npmjs.org/@jsr%2Fbradenmacdonald__s3-lite-client: Not Found - 404

maybe i am doing something wrong. for context my IDE node language server is node 20, despite cloudfare workers not using node. not sure if there's a way to configure IDE for v8 isolate resolution.

but in any event, thanks to both you for supporting s3 on the edge!

riderx · 2024-10-19T13:45:35Z

It seems your pnpm config is incorrect it still try to fetch from npm not jsr.
My PR is not needed anymore cloudflare did improve a lot lately they node support if you use October release it should be good

riderx added 5 commits February 13, 2024 01:49

fix: remove the only dep to use native Uint8Array as recommended here h…

8cad65a

…ttps://sindresorhus.com/blog/goodbye-nodejs-buffer

feat: add npm build

c33ee9d

fix: add gitignore npm folder

8cbad31

fix: use the name of the author for the Pr

162238a

fix: only shim tests

44908aa

bradenmacdonald reviewed Feb 13, 2024

View reviewed changes

riderx added 5 commits February 16, 2024 15:27

fix: use better type

b216061

fix: rollback deps change

9449e0d

fix: make test integration pass in nodejs

fee6b0d

fix: lint and fmt

81b931b

fix: npm build and test

c50b5ca

fix: simplify the problem and fix the tests

8cbf075

fix: lint issue

1835840

fix: use optional shiming like oak

811be45

This was referenced Feb 18, 2024

Try to get oak running under dnt denoland/dnt#14

Closed

fix: add node prefix for util to fix issue in env with min version node 18 nodejs/undici#2775

Merged

bradenmacdonald reviewed Feb 21, 2024

View reviewed changes

bradenmacdonald closed this May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the package compatible Node.js and others runtimes #30

Make the package compatible Node.js and others runtimes #30

riderx commented Feb 13, 2024 •

edited

Loading

riderx commented Feb 13, 2024

bradenmacdonald left a comment

bradenmacdonald Feb 13, 2024

riderx Feb 16, 2024

bradenmacdonald Feb 13, 2024

riderx Feb 16, 2024

bradenmacdonald Feb 13, 2024

riderx Feb 16, 2024 •

edited

Loading

bradenmacdonald Feb 16, 2024

riderx Feb 16, 2024

riderx Feb 16, 2024

riderx Feb 17, 2024

bradenmacdonald commented Feb 13, 2024

riderx commented Feb 17, 2024

riderx commented Feb 17, 2024

riderx commented Feb 18, 2024 •

edited

Loading

riderx commented Feb 18, 2024

riderx commented Feb 18, 2024

riderx commented Feb 18, 2024 •

edited

Loading

riderx commented Feb 18, 2024

riderx commented Feb 18, 2024

bradenmacdonald left a comment

bradenmacdonald Feb 21, 2024

riderx Feb 24, 2024

riderx commented Feb 24, 2024

bradenmacdonald commented Mar 3, 2024

riderx commented Mar 5, 2024

riderx commented Mar 6, 2024

bradenmacdonald commented Mar 6, 2024

riderx commented Mar 6, 2024

bradenmacdonald commented May 1, 2024

willsmanley commented Oct 19, 2024 •

edited

Loading

riderx commented Oct 19, 2024

	let intervalTimer: any;
	let intervalTimer: ReturnType<typeof setTimeout>;

Make the package compatible Node.js and others runtimes #30

Make the package compatible Node.js and others runtimes #30

Conversation

riderx commented Feb 13, 2024 • edited Loading

riderx commented Feb 13, 2024

bradenmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riderx Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Feb 13, 2024

riderx commented Feb 17, 2024

riderx commented Feb 17, 2024

riderx commented Feb 18, 2024 • edited Loading

riderx commented Feb 18, 2024

riderx commented Feb 18, 2024

riderx commented Feb 18, 2024 • edited Loading

riderx commented Feb 18, 2024

riderx commented Feb 18, 2024

bradenmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riderx commented Feb 24, 2024

bradenmacdonald commented Mar 3, 2024

riderx commented Mar 5, 2024

riderx commented Mar 6, 2024

bradenmacdonald commented Mar 6, 2024

riderx commented Mar 6, 2024

bradenmacdonald commented May 1, 2024

willsmanley commented Oct 19, 2024 • edited Loading

riderx commented Oct 19, 2024

riderx commented Feb 13, 2024 •

edited

Loading

riderx Feb 16, 2024 •

edited

Loading

riderx commented Feb 18, 2024 •

edited

Loading

riderx commented Feb 18, 2024 •

edited

Loading

willsmanley commented Oct 19, 2024 •

edited

Loading